Convolutions of Images 2d Python implementation #819

rzalawad · 2021-07-19T12:28:05Z

No description provided.

… added python code.

Amaras

Alright, thanks for the PR!
I left a few comments, you may or may not take into account, and that's fine.
Overall, it seems like a faithful port of the Julia code, so no major problem there. I still hate the 4 live indices in the convolution, but it is not your fault, so the comment about the 4 loops is not that relevant.

However, there is a blocking point, which is that the y /= np.linalg.norm (line 84) won't run because of a type error.
You need to correct that before we can merge it in the AAA.

I think you have not run your code, which is why this bug is here and has slipped under your radar.

But once again, thanks for your interest in the AAA! 😃

Amaras · 2021-08-16T19:05:16Z

contents/convolutions/code/python/2d_convolution.py

+    for i in range(output_size[0]):
+        for j in range(output_size[1]):
+            for k in range(max(0, i-filter.shape[0]), i+1):
+                for l in range(max(0, j-filter.shape[1]), j+1):


I hate the fact that there are four indices and ranges of maximums, but I don't think there is any other way to do that and not be confusing.

Amaras · 2021-08-16T19:07:30Z

contents/convolutions/code/python/2d_convolution.py

+        for j in range(output_size[1]):
+            for k in range(max(0, i-filter.shape[0]), i+1):
+                for l in range(max(0, j-filter.shape[1]), j+1):
+                    if k < signal.shape[0] and i-k < filter.shape[0] \


Please you parentheses for continuation, backslash continuation should only be used for context managers before Python 3.10 (because of syntax restrictions)

Suggested change

if k < signal.shape[0] and i-k < filter.shape[0] \

if (k < signal.shape[0] and i-k < filter.shape[0]

contents/convolutions/code/python/2d_convolution.py

Amaras · 2021-08-16T19:16:28Z

contents/convolutions/code/python/2d_convolution.py

@@ -0,0 +1,129 @@
+import numpy as np
+import math


You don't need to import math all the useful functions are available (and vectorized) in the numpy namespace

Amaras · 2021-08-16T19:18:56Z

contents/convolutions/code/python/2d_convolution.py

+def compute_sobel(signal):
+    Sx, Sy = create_sobel_operators()
+
+    Gx = convolve_linear(signal, Sx, tuple(map(sum,


Okay, passing the shape argument is awkward... There should be a way to do that more simply, right?
I think an auxiliary function would be better here.

Amaras · 2021-08-16T19:21:13Z

contents/convolutions/code/python/2d_convolution.py

+    # Gaussian signals
+    y = [[math.exp(-(((i-50)/100) ** 2 +
+                     ((j-50)/100) ** 2) / .01) for j in range(100)]
+         for i in range(100)]


There should probably be a native numpy way to create that array, rather than rely on a double list comprehension...

Amaras · 2021-08-16T19:23:07Z

contents/convolutions/code/python/2d_convolution.py

+
+    # Normalization is not strictly necessary, but good practice
+    x = x / np.linalg.norm(x)
+    y = y / np.linalg.norm(y)


That won't run: y is a list, not a numpy array. You need to make it an array before using the / operator on it.

You are right that y is a list. When I run the code, it runs (and results in the correct output) and y is casted too ndarray.

Amaras · 2021-08-16T19:28:17Z

contents/convolutions/code/python/2d_convolution.py

+         for i in range(100)]
+
+    # Normalization is not strictly necessary, but good practice
+    x = x / np.linalg.norm(x)


You can simply do:

Suggested change

x = x / np.linalg.norm(x)

x /= np.linalg.norm(x)

Amaras · 2021-08-16T19:33:19Z

contents/convolutions/code/python/2d_convolution.py

+            for k in range(max(0, i-filter.shape[0]), i+1):
+                for l in range(max(0, j-filter.shape[1]), j+1):
+                    if k < signal.shape[0] and i-k < filter.shape[0] \
+                            and l < signal.shape[1] and j-l < filter.shape[1]:


Suggested change

and l < signal.shape[1] and j-l < filter.shape[1]:

and l < signal.shape[1] and j-l < filter.shape[1]):

``` (following the previous comment)

contents/convolutions/code/python/2d_convolution.py

+                for l in range(max(0, j-filter.shape[1]), j+1):
+                    if k < signal.shape[0] and i-k < filter.shape[0] \
+                            and l < signal.shape[1] and j-l < filter.shape[1]:
+                        sum += signal[k, l] * filter[i-k, j-l]


…to not use escape character

rzalawad · 2021-08-16T22:00:11Z

@Amaras I updated the if conditions to use suppress. Thanks for that suggestion. Code looks much cleaner. I also updated the if statements to not use backslashes, and use the from function for the cases where a gaussian kernel needs to be created. I ran the code and tested it by using a common input and checking the output of the Julia code and my code. The line y = y / np.linalg.norm(y) works (doesn't raise an error). I am unsure why that is the case. If it raises an error on your end, please let me know what I can do to replicate the issue.

leios · 2021-08-17T03:52:36Z

At this stage, I don't fully understand why, but here is the output of your "full_linear.dat" file:

Here is the one from Julia:

leios · 2021-08-17T03:53:46Z

I can dig into it in a bit, but this will need to be fixed before merging.

Thanks again for the submission!

rzalawad · 2021-08-17T20:14:22Z

@leios Thank you for catching this. I would prefer to dig into it and fix it. Do the other outputs look correct? Also, what program are you using to visualize the output?

leios · 2021-08-17T22:04:28Z

All image files are output so you can visualize them quickly with gnuplot (hence why we have an entire chapter on plotting for the data in the algorithm archive). Here, it should just be:

set view map
splot "out.dat" matrix with image

The simple linear option also looks incorrect, so far as I could see.

Amaras · 2021-08-18T07:10:55Z

contents/convolutions/code/python/2d_convolution.py

+    x /= np.linalg.norm(x)
+    y /= np.linalg.norm(y)
+
+    x = np.array([[5.0, 3.0], [4.0, 2.0]])


You forgot to do something with the previous x and y

Amaras · 2021-08-18T07:17:25Z

@Ridham177

The line y = y / np.linalg.norm(y) works (doesn't raise an error). I am unsure why that is the case.

It shouldn't raise any issue anymore, as you created a numpy array with y = np.from_function(...)
I think you ran it with the new version, which is why it doesn't raise an error, and not the previous one. If you checkout your previous version and try to run it, it should crash with a TypeError, with something like "Unimplemented operation '/' for types 'list', 'float'."

@leios

At this stage, I don't fully understand why, but here is the output of your "full_linear.dat" file:

I think that goes with my previous comment.

rzalawad · 2021-08-18T18:26:55Z

@leios The output from the julia code now matches the output of the python code. Thank you for reviewing this.

rzalawad · 2021-08-18T18:43:02Z

@Amaras This video where I try the buggy code using the old version shows that it doesn't raise error, probably because of python magic. With that being said, the old version shouldn't be used because its not clean and the types are not the same. Thanks for reviewing and for improving the code.

Amaras · 2021-08-18T22:09:54Z

@Amaras This video where I try the buggy code using the old version shows that it doesn't raise error, probably because of python magic. With that being said, the old version shouldn't be used because its not clean and the types are not the same. Thanks for reviewing and for improving the code.

That probably should not happen, but hey! That's numpy magic, rather than Python magic.

Amaras

Now that the code is much cleaner, clearer, and runs like the Python code, I don't have anything against it.
The create_circle could be a bit clearer, but I don't care that much for it.
That's good enough for me, @leios you can probably merge after the action builds.

Amaras · 2021-08-18T22:14:05Z

contents/convolutions/code/python/2d_convolution.py

+            y_position = ((j * grid_extents / image_resolution)
+                          - 0.5 * grid_extents)
+            if x_position ** 2 + y_position ** 2 <= radius ** 2:
+                out[i, j] = 1.0


There should probably be another way to do that one, but that not really that necessary to implement a clearer way.

leios

Just fixing the line numbers, but while running this, I noticed something odd

[leios@noema julia]$ time julia 2d_convolution.jl 

real    0m1.311s
user    0m1.256s
sys     0m0.053s

[leios@noema python]$ time python 2d_convolution.py 

real    3m18.118s
user    3m17.988s
sys     0m0.017s

Python is 151x slower?

Should we lower the resolution of the output image or maybe consider using numpy? Neither are necessary. I am happy enough with the code producing the correct output, but looking forward, we are thinking about having an overlying testing framework for a lot of the AAA and a 3 minute test is a bit long.

contents/convolutions/2d/2d.md

Co-authored-by: James Schloss <jrs.schloss@gmail.com>

rzalawad · 2021-08-19T00:34:21Z

@leios If we use numpy, it would defeat the purpose of the explicit programming. Most things would be done by numpy and wouldn't be visible in the program. We can reduce the signal dimensions to 10,10 instead of 100, 100. However, modifications to other aspects and to julia code would also be required.

leios

This looks good, thanks for the review and code!

leios · 2021-08-23T04:21:17Z

Happy enough with this, but because you are a first-time contributor, feel free to put your name in the CONTRIBUTORS.md file as well! You don't have to if you do not want to, but I wanted to make sure you knew that was a thing!

…rithm-archive into convolutions_2d_in_python

Amaras · 2021-08-23T22:00:41Z

Please resolve the conflict on CONTRIBUTORS.md before we can merge your PR, it's the only thing left to do (your branch seems to be late with respect to master)

rzalawad · 2021-08-24T00:06:49Z

@Amaras I was lost when fixing this. Please let me know if the fix is correct.

leios

This looks good to me, Thanks a bunch!

Added python code for convolutions 2d. Modified 2d.md file to include…

aa16911

… added python code.

rzalawad changed the title ~~Added python code for convolutions 2d. Modified 2d.md file to include…~~ Add python implementation for convolutions 2d Jul 19, 2021

rzalawad changed the title ~~Add python implementation for convolutions 2d~~ Convolutions of Images 2d Python implementation Jul 19, 2021

Amaras requested changes Aug 16, 2021

View reviewed changes

Updated list comprehension code to numpy code. Changed if statements …

bc3583f

…to not use escape character

rzalawad added 2 commits August 16, 2021 15:02

fixed minor errors

2c99b9f

solved minor errors

2180a0d

Amaras reviewed Aug 18, 2021

View reviewed changes

Changed circle size. Removed test values for x and y

31e4c19

Amaras approved these changes Aug 18, 2021

View reviewed changes

leios reviewed Aug 18, 2021

View reviewed changes

contents/convolutions/2d/2d.md Outdated Show resolved Hide resolved

contents/convolutions/2d/2d.md Outdated Show resolved Hide resolved

rzalawad and others added 2 commits August 18, 2021 17:31

Update contents/convolutions/2d/2d.md

f8082c7

Co-authored-by: James Schloss <jrs.schloss@gmail.com>

Update contents/convolutions/2d/2d.md

9f8f60a

Co-authored-by: James Schloss <jrs.schloss@gmail.com>

Amaras added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Aug 21, 2021

leios approved these changes Aug 23, 2021

View reviewed changes

rzalawad added 2 commits August 23, 2021 09:07

added name to contributors.md

da087f8

Merge branch 'convolutions_2d_in_python' of github.com:Ridham177/algo…

8659d17

…rithm-archive into convolutions_2d_in_python

resolved conflict

5718536

rzalawad added 3 commits August 23, 2021 17:01

removed name from contributors.md. will add next commit

c1ad3c0

added name to contributors.md

023c5b9

added name

17cf36d

leios approved these changes Aug 24, 2021

View reviewed changes

leios merged commit 841cb11 into algorithm-archivists:master Aug 24, 2021

	if k < signal.shape[0] and i-k < filter.shape[0] \
	if (k < signal.shape[0] and i-k < filter.shape[0]

	and l < signal.shape[1] and j-l < filter.shape[1]:
	and l < signal.shape[1] and j-l < filter.shape[1]):
	``` (following the previous comment)

Uh oh!

Convolutions of Images 2d Python implementation #819

Convolutions of Images 2d Python implementation #819

Uh oh!

Conversation

rzalawad commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amaras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

rzalawad commented Aug 16, 2021

Uh oh!

leios commented Aug 17, 2021

Uh oh!

leios commented Aug 17, 2021

Uh oh!

rzalawad commented Aug 17, 2021

Uh oh!

leios commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amaras commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzalawad commented Aug 18, 2021

Uh oh!

rzalawad commented Aug 18, 2021

Uh oh!

Amaras commented Aug 18, 2021

Uh oh!

Amaras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzalawad commented Aug 19, 2021

Uh oh!

leios left a comment

Choose a reason for hiding this comment

Uh oh!

leios commented Aug 23, 2021

Uh oh!

Amaras commented Aug 23, 2021

Uh oh!

rzalawad commented Aug 24, 2021

Uh oh!

leios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rzalawad commented Jul 19, 2021 •

edited

Loading

Amaras commented Aug 18, 2021 •

edited

Loading